Skip to content

Fix vm disk remove #252

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Jul 7, 2023
Merged

Fix vm disk remove #252

merged 4 commits into from
Jul 7, 2023

Conversation

@justinc1
Copy link
Collaborator

@justinc1 justinc1 commented Jul 3, 2023

Sometimes we can remove disk from a running VM, and sometimes we need to shutdown VM before disk can be removed. It depends on HyperCore thinking VM might be using the disk.

The PR:

  • first tries to remove disk from a running VM (the behaviour before this PR)
  • if this fails, it shutdowns VM, tries delete a second time and start VM back

Integration tests for all this is included, for vm and vm_disk modules. It covers 3 cases:

  • VM being off
  • VM running and cannot be shutdown (it does not respond to nice shutdown, and force shutdown is not allowed)
  • VM running and can be shutdown (force shutdown is allowed)

A small bug in vm_disk module was also fixed. The module didn't start back VM if it was shutdown during execution.

@justinc1 justinc1 requested review from PolonaM and shoriminimoe July 3, 2023 08:09
@justinc1 justinc1 force-pushed the fix-vm-disk-remove branch 2 times, most recently from 0ca1640 to 4988468 Compare July 3, 2023 08:32
@justinc1
Copy link
Collaborator Author

justinc1 commented Jul 3, 2023 •
edited
Loading

Integ test: https://github.com/ScaleComputing/HyperCoreAnsibleCollection/actions/runs/5464904452 https://github.com/ScaleComputing/HyperCoreAnsibleCollection/actions/runs/5479912287

Edit: in 5479912287 failed vm_clone, looks like a bug. A failed virtual_disk_attach is likely not a bug in collection (but in test).

shoriminimoe
shoriminimoe approved these changes Jul 5, 2023
View reviewed changes
Copy link
Collaborator

@shoriminimoe shoriminimoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still learning Ansible testing so this may be an ignorant question. Are the new vm__* and vm_disk__* tests not the same thing? What is the difference?

@justinc1 justinc1 force-pushed the fix-vm-disk-remove branch from 58b7aa4 to 6f3a752 Compare July 6, 2023 20:33
@justinc1
Copy link
Collaborator Author

justinc1 commented Jul 6, 2023

Are the new vm__* and vm_disk__* tests not the same thing?

They are very similar, and both modules use same code from module_utils. But they are not exactly the same, so I added separate tests (and after doing this, I noticed vm_disk module does not correctly restart VMs).

@justinc1 justinc1 force-pushed the fix-vm-disk-remove branch from 6f3a752 to a760aa7 Compare July 6, 2023 20:59
@justinc1
CI test disk remove from VM
cff844f 
Signed-off-by: Justin Cinkelj <justin.cinkelj@xlab.si>
@justinc1 justinc1 force-pushed the fix-vm-disk-remove branch from a760aa7 to 8e92668 Compare July 7, 2023 07:12
PolonaM
PolonaM reviewed Jul 7, 2023
View reviewed changes
PolonaM
PolonaM approved these changes Jul 7, 2023
View reviewed changes
Copy link
Collaborator

@PolonaM PolonaM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Justin for the work! Except for the mentioned replacement of task_tag with task_status and maybe even adding the task status to typed_classes.py, everything else looks good to me.

justinc1 added 3 commits July 7, 2023 13:49
@justinc1
Shutdown VM if disk cannot be removed from running VM
714c561 
Before VM was (always) shutdown only if IDE CD-ROM was removed.
Now VM will be shutdown if disk removal from running VM fails.

Fixes #249

Signed-off-by: Justin Cinkelj <justin.cinkelj@xlab.si>
@justinc1
Fix missing VM start in vm_disk module
34e634f 
Flag .needs_reboot was set on one object, but vm_power_up() was called on
different object, so VM was not started back.

Signed-off-by: Justin Cinkelj <justin.cinkelj@xlab.si>
@justinc1
CI test disk remove and VM reboot for all disk types
e5297aa 
Signed-off-by: Justin Cinkelj <justin.cinkelj@xlab.si>
@justinc1 justinc1 force-pushed the fix-vm-disk-remove branch from 31fc75f to e5297aa Compare July 7, 2023 11:50
@justinc1 justinc1 merged commit 629e853 into main Jul 7, 2023
@justinc1 justinc1 deleted the fix-vm-disk-remove branch July 7, 2023 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shoriminimoe shoriminimoe shoriminimoe approved these changes

@PolonaM PolonaM PolonaM approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@justinc1 @shoriminimoe @PolonaM